fix: slow start config add min_weight_percent field to avoid too big edf deadline by jiangshantao-dbg · Pull Request #19712 · envoyproxy/envoy

jiangshantao-dbg · 2022-01-27T02:43:06Z

PR to fix #19526

Commit Message:
cluster SlowStartConfig add min_weight_percent field to avoid too big edf deadline. set default value 0.1.

Additional Description:
Risk Level: Low
Testing: Done
Docs Changes: Done
Release Notes:Done
Fixes #19526

repokitteh-read-only · 2022-01-27T02:43:09Z

Hi @jiangshantao-dbg, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19712 was opened by jiangshantao-dbg.

see: more, trace.

repokitteh-read-only · 2022-01-27T02:43:14Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19712 was opened by jiangshantao-dbg.

see: more, trace.

adisuissa

/lgtm api
Looked briefly at the code, and left a comment.
Consider adding tests for the minimal (0.0) and maximal (100.0) values.
/wait

adisuissa · 2022-01-27T19:55:32Z

source/common/upstream/load_balancer_impl.cc

Technically this will be checked by the PGV validator (you added a constraint to the field that automatically checks that). Also this doesn't take into account equals on both ends.
Better to remove the checks and let PGV validate that.

what if remove the PGV validator for this field, just use the constructor to init the value. It will throw an exception if the PGV failed, which cause the cluster can't be adding to manager. I think the min_weight_percent should not affect the cluster update, so just the warning message and use the default value 0.1 is enough ?

Also the min_weight_percent can't be 0 which will cause a really big edf deadline, and we want use the default value if the user not specify a value. so the constraint in proto use gte: 0.0(pb default value 0.0) and the constructor use > 0.0.

PGV is meant to detect problems early and reject configs that are incorrect, so I encourage using that.

If min_weight_percent must be greater than 0.0, why not use gt instead of gte? Same goes for the upper bound (see here for more info)

PING?
/wait

sorry, these days i am in a vacation. i will use {lt: 1.0 gt: 0.0 ignore_empty: true} to the field min_weight_percent, which allows users has not to specify the default value. and will keep checking logic in constructor to ensure the default value to 0.1.

adisuissa · 2022-01-27T19:58:47Z

source/common/upstream/load_balancer_impl.cc

Note that if PGV rejects the config (due to wrong value), this code will never be reached.

adisuissa · 2022-02-03T02:14:47Z

Please let me know what you think about changing the PGV constraint (using lt and gt).
/wait

…edf deadline Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

jiangshantao-dbg · 2022-02-07T09:39:28Z

/retest

repokitteh-read-only · 2022-02-07T09:39:32Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19712 (comment) was created by @jiangshantao-dbg.

see: more, trace.

jiangshantao-dbg · 2022-02-08T07:40:09Z

Please let me know what you think about changing the PGV constraint (using lt and gt). /wait

@adisuissa sorry, i rebase the branch which cause the reviewing processing reset. i have completed the changes, and all test cases are ok. please have a look.

adisuissa

Thanks for the update.
Looked again at the API style guide, and left a comment to what I think should be changed here.
/wait

adisuissa · 2022-02-08T14:23:04Z

api/envoy/config/cluster/v3/cluster.proto

+    // Configures the minimum percentage of origin weight that avoids too small new weight,
+    // which may cause endpoints in slow start mode receive no traffic in slow start window.
+    // Range between (0.0, 1.0). If not specified, the default is 0.1.
+    double min_weight_percent = 3 [(validate.rules).double = {lt: 1.0 gt: 0.0 ignore_empty: true}];


The type should be changed to Percent (see api style).
Also note that this implies that the range should be (0, 100).

Another option is to use a double wrapper (as the default value isn't 0), and then use PROTOBUF_GET_WRAPPED_OR_DEFAULT to set the value in the c'tor.

adisuissa · 2022-02-08T14:27:45Z

source/common/upstream/load_balancer_impl.cc

+  min_weight_percent_ = 0.1;
+  if (slow_start_config.has_value()) {
+    if (slow_start_config.value().min_weight_percent() > 0.0 &&
+        slow_start_config.value().min_weight_percent() < 1.0) {
+      min_weight_percent_ = slow_start_config.value().min_weight_percent();
+    } else {
+      ENVOY_LOG(warn,
+                "Invalid value {} provided for min_weight_percent parameter, must be in range "
+                "(0.0, 1.0), so use default value 0.1",
+                slow_start_config.value().min_weight_percent());
+    }
+  }


Once the field is of type Percent, you can initialize the min_weight_percent_ field in the MIL, and use PROTOBUF_PERCENT_TO_ROUNDED_INTEGER_OR_DEFAULT

Something similar to:

PROTOBUF_PERCENT_TO_DOUBLE_OR_DEFAULT( slow_start_config, min_weight_percent, 10))/100.0

thanks for advice. i have change to use Percent. please have a look.

jiangshantao-dbg · 2022-02-15T06:33:33Z

This is the fix suggested in #19526, so the approach LGTM. I had a few nits in the comments, but overall the docs/code are great.

One thing unrelated to the code that I would suggest is to elaborate on the problem this PR is solving. The release note doesn't make it clear why this was necessary and it would just make it easier for anyone who's curious to quickly get an answer.

Thank you for doing this!

Thank for you advise. i has fix code/docs as the comments suggests. please have a look. @tonya11en

jiangshantao-dbg · 2022-02-18T02:33:12Z

/retest

repokitteh-read-only · 2022-02-18T02:33:15Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19712 (comment) was created by @jiangshantao-dbg.

see: more, trace.

tonya11en

Thanks, LGTM!

rojkov · 2022-02-28T08:47:50Z

/wait
on merge conflict

…aline

jiangshantao-dbg · 2022-02-28T09:09:39Z

/wait on merge conflict

@rojkov conflict resolved

rojkov · 2022-02-28T11:27:10Z

Sorry, you might need to merge the main branch again as it had been broken for some time.

…t-too-big-edf-dealine Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

jiangshantao-dbg · 2022-03-01T02:02:02Z

Sorry, you might need to merge the main branch again as it had been broken for some time.

@rojkov done

rojkov · 2022-03-04T08:45:49Z

@tonya11en could you reinstate your approval?

jiangshantao-dbg · 2022-03-07T02:11:50Z

@adisuissa could you review this pr again. there is 1 workflow awaiting approval. thanks

adisuissa

Thanks!
/lgtm api
Left a couple of nits, but otherwise LGTM.

adisuissa · 2022-03-08T14:16:50Z

test/common/upstream/load_balancer_impl_test.cc

    return std::chrono::time_point_cast<std::chrono::milliseconds>(edf_lb.latest_host_added_time_)
        .time_since_epoch();
  }
+  static double slowStartMinWeightPercent(EdfLoadBalancerBase& edf_lb) {


nit:

Suggested change

static double slowStartMinWeightPercent(EdfLoadBalancerBase& edf_lb) {

static double slowStartMinWeightPercent(const EdfLoadBalancerBase& edf_lb) {

adisuissa · 2022-03-08T14:17:47Z

source/common/upstream/load_balancer_impl.h

  const absl::optional<Runtime::Double> aggression_runtime_;
  TimeSource& time_source_;
  MonotonicTime latest_host_added_time_;
+  double slow_start_min_weight_percent_;


Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

jiangshantao-dbg · 2022-03-10T02:01:29Z

/retest

repokitteh-read-only · 2022-03-10T02:01:32Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19712 (comment) was created by @jiangshantao-dbg.

see: more, trace.

adisuissa · 2022-03-10T03:57:20Z

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2022-03-10T03:57:23Z

@envoyproxy/senior-maintainers assignee is @yanavlasov

🐱

Caused by: a #19712 (comment) was created by @adisuissa.

see: more, trace.

…edf deadline (envoyproxy#19712) * fix: slow start config add min_weight_percent field to avoid too big edf deadline Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

* fix: slow start config add min_weight_percent field to avoid too big edf deadline (envoyproxy#19712) * fix: slow start config add min_weight_percent field to avoid too big edf deadline Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com> * fix: ./tools/proto_format/proto_format.sh fix generate proto Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com> Co-authored-by: jiangshantao <jiangshantao-dbg@qq.com>

…edf deadline (envoyproxy#19712) * fix: slow start config add min_weight_percent field to avoid too big edf deadline Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

repokitteh-read-only bot added the api label Jan 27, 2022

repokitteh-read-only bot assigned adisuissa Jan 27, 2022

jiangshantao-dbg force-pushed the fix/slow-start-formula-result-too-big-edf-dealine branch 2 times, most recently from d99d9be to eb5a7e5 Compare January 27, 2022 06:58

adisuissa requested changes Jan 27, 2022

View reviewed changes

repokitteh-read-only bot added waiting api and removed api waiting labels Jan 27, 2022

repokitteh-read-only bot added waiting and removed waiting labels Feb 3, 2022

jiangshantao added 10 commits February 7, 2022 11:26

fix: slow start config add min_weight_percent field to avoid too big …

c9fa80c

…edf deadline Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

docs: fix double quote

7db579d

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

docs: fix version history not in alphabetical order

55fa090

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

fix: fix syntax error of std::

fa05185

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

fix: fix syntax error of missing ;

6bcae63

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

fix: fix test syntax error

c978af2

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

fix: fix the test error and percent range

f5f2e97

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

fix: fix PVG rules for min_weight_percent and add test case

730edc0

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

fix: remove tailer whitespace

a4d0305

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

fix: rebase main and fix current.rst

929bd47

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

jiangshantao-dbg force-pushed the fix/slow-start-formula-result-too-big-edf-dealine branch from 532a2d2 to 929bd47 Compare February 7, 2022 03:36

fix: fix format error

08201dd

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

adisuissa reviewed Feb 8, 2022

View reviewed changes

tonya11en previously approved these changes Feb 21, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 28, 2022

Merge branch 'main' into fix/slow-start-formula-result-too-big-edf-de…

228879d

…aline

jiangshantao-dbg dismissed tonya11en’s stale review via 228879d February 28, 2022 09:08

repokitteh-read-only bot removed the waiting label Feb 28, 2022

Merge remote-tracking branch 'main' into fix/slow-start-formula-resul…

226e9cf

…t-too-big-edf-dealine Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

tonya11en previously approved these changes Mar 4, 2022

View reviewed changes

adisuissa requested changes Mar 8, 2022

View reviewed changes

fix: code nits fixed

8a205ba

Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

jiangshantao-dbg dismissed tonya11en’s stale review via 8a205ba March 9, 2022 06:52

repokitteh-read-only bot assigned yanavlasov Mar 10, 2022

adisuissa approved these changes Mar 10, 2022

View reviewed changes

yanavlasov approved these changes Mar 10, 2022

View reviewed changes

yanavlasov merged commit e3fede5 into envoyproxy:main Mar 10, 2022

	static double slowStartMinWeightPercent(EdfLoadBalancerBase& edf_lb) {
	static double slowStartMinWeightPercent(const EdfLoadBalancerBase& edf_lb) {

Conversation

jiangshantao-dbg commented Jan 27, 2022

Uh oh!

repokitteh-read-only bot commented Jan 27, 2022

Uh oh!

repokitteh-read-only bot commented Jan 27, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Feb 3, 2022

Uh oh!

jiangshantao-dbg commented Feb 7, 2022

Uh oh!

repokitteh-read-only bot commented Feb 7, 2022

Uh oh!

jiangshantao-dbg commented Feb 8, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangshantao-dbg commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiangshantao-dbg commented Feb 18, 2022

Uh oh!

repokitteh-read-only bot commented Feb 18, 2022

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

rojkov commented Feb 28, 2022

Uh oh!

jiangshantao-dbg commented Feb 28, 2022

Uh oh!

rojkov commented Feb 28, 2022

Uh oh!

jiangshantao-dbg commented Mar 1, 2022

Uh oh!

rojkov commented Mar 4, 2022

Uh oh!

jiangshantao-dbg commented Mar 7, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangshantao-dbg commented Mar 10, 2022

Uh oh!

repokitteh-read-only bot commented Mar 10, 2022

Uh oh!

adisuissa Feb 3, 2022 •

edited

Loading

jiangshantao-dbg commented Feb 15, 2022 •

edited

Loading